feat: use cronos store #1895
feat: use cronos store #1895
Conversation
songgaoye
requested review from
calvinaco and
thomas-nguy
and removed request for
a team
This comment has been minimized.
This comment has been minimized.
WalkthroughRemoved in-repo memiavl/versiondb/store implementations, related tools/tests, and memiavl wiring; replaced internal modules and imports with external github.com/crypto-org-chain/cronos-store modules; updated app callsite to pass chainId to memiavl setup; adjusted proto go_package options and proto-gen script; consolidated Makefile/CI/test flows. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 checkmake (0.2.2)Makefile[warning] 344-344: Missing required phony target "all" (minphony) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Can you change the PR target to be |
songgaoye
force-pushed
the
song/use_cronos_store
branch
from
77721c2 to
aff8700
Compare
|
sure |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(3 hunks)gomod2nix.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: Run golangci-lint
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: gomod2nix
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
gomod2nix.toml (1)
259-270: ****The claim of a version mismatch is inaccurate. Go.mod contains a
replacedirective that explicitly redirectsgithub.com/crypto-org-chain/cronos-store/memiavlfrom the constraintv0.0.4to the replacement versionv0.0.0-20251119030934-19a930cec475, which matches the gomod2nix.toml entries. Replace directives take precedence in Go module resolution—no mismatch exists and no correction is needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/app.go (1)
433-434: Consider adding validation to ensure chainId is provided before passing to SetupMemIAVL.The
chainIdparameter is retrieved from application options but passed directly toSetupMemIAVLwithout validation. While the standard pattern in Cosmos SDK apps retrieves config values this way, adding a check to ensurechainIdis non-empty would provide defensive validation, particularly in edge cases or misconfigured environments where the flag might not be set:chainId := cast.ToString(appOpts.Get(flags.FlagChainID)) +if chainId == "" { + panic("chainId must be provided for memiavl setup") +} baseAppOptions = memiavlstore.SetupMemIAVL(logger, homePath, appOpts, false, false, cacheSize, chainId, baseAppOptions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)app/app.go(2 hunks)cmd/cronosd/cmd/root.go(1 hunks)go.mod(3 hunks)gomod2nix.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- gomod2nix.toml
- cmd/cronosd/cmd/root.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: Analyze (go)
- GitHub Check: Run golangci-lint
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
app/app.go (1)
44-44: External module dependency is properly declared and versioned.The import path change to
github.com/crypto-org-chain/cronos-store/storeis correctly reflected ingo.modwith versionv0.0.0-20251119030934-19a930cec475. The external dependency is consistently used across the codebase and properly maintained.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
329-331: Consider adding vulncheck to a .PHONY declaration.The vulncheck target is functionally correct, but lacks a
.PHONYdeclaration. While this doesn't affect functionality (since the directory dependency makes it safe), explicitly marking it as.PHONYwould improve clarity and prevent accidental issues if avulncheckfile is ever created in the repository.vulncheck: $(BUILDDIR)/ GOBIN=$(BUILDDIR) go install golang.org/x/vuln/cmd/govulncheck@latest $(BUILDDIR)/govulncheck ./... + +.PHONY: vulncheck
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(2 hunks)Makefile(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: build (macos-latest)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
Makefile (2)
112-113: Consolidated test target looks good.The unified
testtarget appropriately consolidates separate submodule test invocations (test-memiavl, test-store, test-versiondb) into a single entry point. The use of-tags=objstoreis consistent with other test targets throughout the Makefile, and coverage profiling setup is correct.
115-115: PHONY declaration correctly updated.The PHONY declaration appropriately removes the now-consolidated submodule test targets. The minimal declaration for core targets is consistent with this file's pattern of grouping PHONY declarations by functional section (see lines 145, 214-222, etc.).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Makefile (1)
105-115: Addallto .PHONY targets for Makefile best practices.The
alltarget at line 105 should be declared as.PHONYto prevent conflicts with files namedallin the repository root. This is flagged by static analysis tools and is a Makefile best practice.Apply this diff to fix:
-all: build +all: build build: check-network print-ledger go.sum @go build -mod=readonly $(BUILD_FLAGS) -o $(BUILDDIR)/cronosd ./cmd/cronosd install: check-network print-ledger go.sum @go install -mod=readonly $(BUILD_FLAGS) ./cmd/cronosd test: @go test -tags=objstore -v -mod=readonly $(PACKAGES) -coverprofile=$(COVERAGE) -covermode=atomic -.PHONY: clean build install test +.PHONY: all clean build install test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Makefile(3 hunks)scripts/protocgen.sh(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 341-341: Missing required phony target "all"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: build (macos-latest)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: unittest
- GitHub Check: gomod2nix
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: integration_tests (ibc)
- GitHub Check: Run golangci-lint
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
Makefile (3)
112-115: Consolidation of test targets is appropriate.The single
testtarget correctly consolidates the removed per-submodule targets (test-memiavl, test-store, test-versiondb) after the extraction of these packages to cronos-store. The test invocation remains correct.
334-338: Proto breaking change checks correctly split between repos.The proto-check-breaking target appropriately separates validation: Cronos protos are checked against cronos main branch (excluding memiavl), while memiavl protos are validated against the cronos-store version derived from go.mod. This aligns with the package extraction.
290-302: The version extraction logic in the Makefile works correctly for the current go.mod state.The sed pattern correctly extracts
v0.0.4from thecronos-store/memiavldependency in go.mod. Since this version contains no hyphen, it is correctly classified as a tag (CRONOS_STORE_REF_TARGET := tag). The logic appropriately handles the fallback to themainbranch if the version extraction returns empty.scripts/protocgen.sh (1)
20-30: Remove concern about cronos-store proto file generation.The memiavl proto files in
proto/memiavl/specifyoption go_package = github.com/crypto-org-chain/cronos-store/memiavl, so the buf generate loop on lines 7–14 automatically generates the cronos-store memiavl files to thegithub.com/crypto-org-chain/cronos-store/memiavl/directory. The sync step on line 29 copies files that are guaranteed to exist after buf generation completes.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
292-308: Verify robustness of CRONOS_STORE_VERSION extraction fallback logic.The version extraction uses multiple fallback methods:
go list -m(line 292), awk on=>lines (line 294), sed on normal go.mod lines (line 297), and finally defaults to "main" (line 306). The awk pattern at line 294 extracts$$NFfrom lines containing=>, which may match local replace paths (e.g.,/local/path) rather than semantic versions. Verify that:
- The intended input format is
github.com/crypto-org-chain/cronos-store/memiavl => v1.0.0(with version suffix), not just a bare path.- The pattern matching at line 297 correctly handles the standard go.mod format without
=>.- The fallback to "main" provides adequate safety if neither extraction succeeds.
Although the fallback chain prevents breakage, clarifying or hardening this logic would improve maintainability.
Consider adding a comment above line 291 documenting the intended extraction precedence and format assumptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(3 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 347-347: Missing required phony target "all"
(minphony)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: build (macos-latest)
- GitHub Check: build (ubuntu-latest)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-14)
- GitHub Check: unittest
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
Makefile (1)
340-344: Proto-check-breaking logic cleanly separates base and memiavl checks.The split between checking Cronos protobufs (excluding memiavl, against main branch) and memiavl protobufs (against cronos-store using derived ref) is well-structured. The use of
CRONOS_STORE_REFandCRONOS_STORE_REF_TARGETcorrectly aligns with the version derivation logic above.
| @cd versiondb; go test -tags=objstore,rocksdb -v -mod=readonly ./... -coverprofile=$(COVERAGE) -covermode=atomic; | ||
|
|
||
| .PHONY: clean build install test test-memiavl test-store test-versiondb | ||
| .PHONY: clean build install test |
There was a problem hiding this comment.
Missing all target in .PHONY declaration.
The all target is defined at line 105 but is not declared as phony at line 115. This can cause Make to treat all as a file target, potentially leading to incorrect behavior.
Apply this diff to fix the .PHONY declaration:
-.PHONY: clean build install test
+.PHONY: all clean build install test📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .PHONY: clean build install test | |
| .PHONY: all clean build install test |
🤖 Prompt for AI Agents
In Makefile around line 115, the .PHONY declaration omits the existing all
target defined at line 105; update the .PHONY line to include all (e.g., add
"all" to the list) so Make treats it as a phony target rather than a file.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Makefile (1)
115-115: Missingalltarget in .PHONY declaration (duplicate of prior comment).The
alltarget defined at line 105 is not included in the .PHONY declaration. This issue was flagged in a previous review but remains unfixed.Apply this diff to fix the .PHONY declaration:
-.PHONY: clean build install test +.PHONY: all clean build install test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(3 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 344-344: Missing required phony target "all"
(minphony)
🪛 GitHub Actions: Protobuf
Makefile
[error] 341-341: Command failed: make proto-check-breaking. Failure: no .proto target files found.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: build (macos-14)
- GitHub Check: gomod2nix
- GitHub Check: build (macos-latest)
- GitHub Check: unittest
- GitHub Check: build (ubuntu-latest)
- GitHub Check: Run golangci-lint
- GitHub Check: integration_tests (gas)
- GitHub Check: integration_tests (slow)
- GitHub Check: integration_tests (mint)
- GitHub Check: integration_tests (ibc_update_client)
- GitHub Check: integration_tests (upgrade)
- GitHub Check: integration_tests (gov)
- GitHub Check: integration_tests (ica)
- GitHub Check: integration_tests (ibc_rly_evm)
- GitHub Check: integration_tests (ibc_timeout)
- GitHub Check: integration_tests (unmarked)
- GitHub Check: integration_tests (ibc)
- GitHub Check: integration_tests (ibc_rly_gas)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Makefile (2)
115-115: Addalltarget to .PHONY declaration.The
alltarget is defined at line 105 but missing from the .PHONY declaration at line 115. This can cause Make to treatallas a file target rather than a phony target.Apply this diff to fix it:
-.PHONY: clean build install test +.PHONY: all clean build install test
292-305: Fix the CRONOS_STORE_VERSION extraction fallback patterns.The
awkpattern at line 294 anchors with^github.com/..., expecting the module to start at the line's beginning. However, go.mod indents entries with tabs, so this pattern will never match. Thesedfallback at line 297 has similar issues and produces incorrect output ("=>") when applied to the replace directive format.Test the version extraction against actual go.mod content before relying on these fallbacks, or simplify the logic to depend primarily on the
go listcommand at line 292, which works correctly.Consider applying this more robust approach:
-CRONOS_STORE_VERSION := $(shell go list -m -f '{{if .Replace}}{{.Replace.Version}}{{else}}{{.Version}}{{end}}' github.com/crypto-org-chain/cronos-store/memiavl 2>/dev/null) -ifeq ($(CRONOS_STORE_VERSION),) - CRONOS_STORE_VERSION := $(shell awk '/^github.com\/crypto-org-chain\/cronos-store\/memiavl[[:space:]]+=>/ {print $$NF; exit}' go.mod) -endif -ifeq ($(CRONOS_STORE_VERSION),) - CRONOS_STORE_VERSION := $(shell sed -n 's/^[[:space:]]*github.com\/crypto-org-chain\/cronos-store\/memiavl[[:space:]]\+\([^[:space:]]\+\).*/\1/p' go.mod | head -n 1) -endif +CRONOS_STORE_VERSION := $(shell go list -m -f '{{if .Replace}}{{.Replace.Version}}{{else}}{{.Version}}{{end}}' github.com/crypto-org-chain/cronos-store/memiavl 2>/dev/null || grep 'cronos-store/memiavl' go.mod | awk '{print $$NF}' | head -n 1)
🧹 Nitpick comments (1)
Makefile (1)
337-341: Proto-check-breaking for memiavl now skips gracefully but consider removing the check entirely.Line 341 now includes
|| echo "Warning: memiavl proto check skipped..."to handle the missing proto/memiavl directory gracefully. While this is an improvement over a hard failure, it still attempts to run a buf check that will always fail on this path.Since proto/memiavl was moved to the external cronos-store module, either remove this check entirely (cleaner approach) or implement a proper guard that checks for directory existence before running buf:
proto-check-breaking: @echo "Checking Cronos protobuf files for breaking changes" @$(protoImage) buf breaking --against $(HTTPS_GIT)#branch=main --exclude-path proto/memiavl - @echo "Checking memiavl protobuf files against cronos-store ref $(CRONOS_STORE_REF)" - @$(protoImage) buf breaking --path proto/memiavl --against $(CRONOS_STORE_GIT)#ref=$(CRONOS_STORE_REF) || echo "Warning: memiavl proto check skipped (files may not exist in cronos-store yet)"Or, if proto definitions still exist in cronos-store and need verification, add a proper directory check:
proto-check-breaking: @echo "Checking Cronos protobuf files for breaking changes" @$(protoImage) buf breaking --against $(HTTPS_GIT)#branch=main --exclude-path proto/memiavl + @if [ -d proto/memiavl ]; then \ + echo "Checking memiavl protobuf files against cronos-store ref $(CRONOS_STORE_REF)"; \ + $(protoImage) buf breaking --path proto/memiavl --against $(CRONOS_STORE_GIT)#ref=$(CRONOS_STORE_REF); \ + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(3 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 344-344: Missing required phony target "all"
(minphony)
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
Chores
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.